Improvement(rpc-server): Add additional logs for `shadow_data_consistency` #93

kobayurii · 2023-07-20T17:06:36Z

#92
In this pr we improve shadow_data_consistency logs.

Data may mismatch in 4 cases:

Both services(read_rpc and near_rpc) have a successful response
if data mismatch we are logging into shadow_data_consistency_successful_responsesboth response objects for future investigation.
read_rpc - failure, near_rpc - successful
we are logging into shadow_data_consistency_read_rpc_failure_response just message about mismatch data without objects. In the future we should be investigate why we got error in the read_rpc.
read_rpc - successful , near_rpc - failure
we are logging into shadow_data_consistency_near_failure_response just message about mismatch data without objects. Expected that all error will be related with network issues.
Both services have a failure response
we are logging into shadow_data_consistency_failure_responses both response objects for future investigation.
Expected we will only have a difference in the error text.

khorolets

I'd like you to add docstrings to the functions you introduce for readability. This is a general request from me.

Also, could you please consider the changes I've proposed? The change would also affect other places, so I'm not commenting on those.

I can't entirely agree with the place for the log message for data consistency. However, I don't have any better suggestions.

rpc-server/src/modules/blocks/utils.rs

telezhnaya

Have you covered the thing that we should give different logs:

when both answers are successful, but they do not match
when one or both answers were not able to be collected properly
?

kobayurii · 2023-07-21T11:16:22Z

Have you covered the thing that we should give different logs:

when both answers are successful, but they do not match

when one or both answers were not able to be collected properly
?

I haven't forgotten. I analyzed the situation and found a case when an error is normal. For example, if the block does not exist then both services return an error and we need to compare these errors and make sure that we return the same error as near_rpc. At the moment I just added the compared objects to the logging, I hope this will improve the situation with the investigation of mismatching.

khorolets

My requests and concerns were addressed. I am approving, though I wouldn't merge until @telezhnaya approves.

telezhnaya

Sorry, but this does not address the thing I've asked for. The initial idea was to simplify the logs and help to identify the cases when both services give successful results, and these results do not match. You haven't covered this.

kobayurii · 2023-07-22T10:23:47Z

Sorry, but this does not address the thing I've asked for. The initial idea was to simplify the logs and help to identify the cases when both services give successful results, and these results do not match. You haven't covered this.

You are right! I lost some moments and I will improve code coming soon. Sorry for that!

khorolets

Thank you for the detailed PR description. I'm leaving some changes request, but I still need to review the PR.
I am thinking about the way ResponseState works. I feel it should be improved, but I can't concentrate right now to tell how.

rpc-server/src/modules/transactions/methods.rs

rpc-server/src/utils.rs

rpc-server/src/modules/blocks/methods.rs

rpc-server/src/modules/blocks/utils.rs

rpc-server/src/utils.rs

rpc-server/src/modules/blocks/utils.rs

telezhnaya

I see no more major issues here.
But, this does not fix #92, please check it's not closed automatically after the merge
Thank you

khorolets · 2023-07-31T08:50:58Z

rpc-server/src/utils.rs

+            // Both services(read_rpc and near_rpc) have a successful response
+            // if data mismatch we are logging into `shadow_data_consistency_successful_responses`
+            // both response objects for future investigation.
+            tracing::warn!(target: "shadow_data_consistency_successful_responses",


Introduction of couple of new targets really bothers me. Can you reuse the shadow_data_consistency we already have and instead define the cause of the warning in the log message itself, please?

khorolets · 2023-07-31T08:54:13Z

rpc-server/src/modules/queries/methods.rs

-        let read_rpc_response_json = match &result {
-            Ok(res) => serde_json::to_value(res),
-            Err(err) => serde_json::to_value(err),
+        let (read_rpc_response_json, response_success) = match &result {


I don't feel response_success is a good name for this variable. Maybe something like is_response_ok (is_ could help identify it's a bool, _ok is closer to what we deal with). What do you guys think @kobayurii @telezhnaya ?

2. Logs inmprovements for shadow_data_consistency. Add Full objects in log if data mismatch

khorolets · 2023-08-02T07:22:14Z

rpc-server/src/utils.rs

+            )
+        };
+        return Err(ShadowDataConsistencyError::ResultsDontMatch(format!(
+            "{err}. Reason: {reason}. Description {description}"


Can you post an example of such input for clarity, please?

Yes! Sure.
exampl.log

khorolets · 2023-08-02T14:59:19Z

@kobayurii @telezhnaya
I've talked to @gmilescu and he suggested us splitting the errors in the counters. Something like:

block_error_proxies_counter_0 - RR success, AR success - data mismatch
block_error_proxies_counter_1 - RR success, AR error
block_error_proxies_counter_3 - RR error, AR success

This is pretty similar to what @kobayurii did in this PR for separating logs. What do you think? I guess we should merge this PR and open a separate followup issue to cover the idea described.

kobayurii requested review from khorolets and telezhnaya July 20, 2023 17:06

kobayurii self-assigned this Jul 20, 2023

khorolets requested changes Jul 21, 2023

View reviewed changes

rpc-server/src/modules/blocks/utils.rs Outdated Show resolved Hide resolved

telezhnaya reviewed Jul 21, 2023

View reviewed changes

kobayurii force-pushed the add_retry branch from 087a313 to 3d73c00 Compare July 21, 2023 10:53

kobayurii requested a review from khorolets July 21, 2023 10:55

kobayurii force-pushed the add_retry branch from 3d73c00 to 3835ee2 Compare July 21, 2023 10:57

kobayurii requested a review from telezhnaya July 21, 2023 11:16

khorolets approved these changes Jul 21, 2023

View reviewed changes

telezhnaya requested changes Jul 21, 2023

View reviewed changes

kobayurii force-pushed the add_retry branch 2 times, most recently from 5a25ede to 3a258b9 Compare July 23, 2023 18:47

kobayurii requested review from telezhnaya and khorolets July 23, 2023 18:50

kobayurii force-pushed the add_retry branch 2 times, most recently from d332da5 to ba203e6 Compare July 23, 2023 19:20

kobayurii changed the title ~~1. Add retry to get objects from s3 bucket~~ Improvement(rpc_server): Add retrying to fetch object from s3 and add additional logs for shadow_data_consistency Jul 24, 2023

khorolets changed the title ~~Improvement(rpc_server): Add retrying to fetch object from s3 and add additional logs for shadow_data_consistency~~ Improvement(rpc-server): Add retrying to fetch object from s3 and add additional logs for shadow_data_consistency Jul 24, 2023

khorolets requested changes Jul 24, 2023

View reviewed changes

rpc-server/src/modules/transactions/methods.rs Outdated Show resolved Hide resolved

rpc-server/src/utils.rs Outdated Show resolved Hide resolved

telezhnaya requested changes Jul 24, 2023

View reviewed changes

kobayurii force-pushed the add_retry branch 2 times, most recently from b37ec6f to 8dc190a Compare July 24, 2023 19:11

kobayurii requested review from khorolets and telezhnaya July 24, 2023 19:25

kobayurii force-pushed the add_retry branch from ec84a33 to 47ac0c0 Compare July 26, 2023 09:06

kobayurii changed the title ~~Improvement(rpc-server): Add retrying to fetch object from s3 and add additional logs for shadow_data_consistency~~ Improvement(rpc-server): Add additional logs for shadow_data_consistency Jul 26, 2023

telezhnaya approved these changes Jul 26, 2023

View reviewed changes

khorolets requested changes Jul 31, 2023

View reviewed changes

kobayurii added 2 commits July 31, 2023 20:30

1. Add retry to get objects from s3 bucket

4161406

2. Logs inmprovements for shadow_data_consistency. Add Full objects in log if data mismatch

rename variables to more clear and improve msg error for mismatch data

3000155

kobayurii force-pushed the add_retry branch from 47ac0c0 to 3000155 Compare July 31, 2023 17:30

kobayurii requested a review from khorolets July 31, 2023 17:35

khorolets reviewed Aug 2, 2023

View reviewed changes

log inmprovement

e71da95

kobayurii requested a review from khorolets August 2, 2023 10:22

khorolets approved these changes Aug 3, 2023

View reviewed changes

khorolets mentioned this pull request Aug 3, 2023

Split error metrics to disambiguate the reason #97

Closed

khorolets merged commit bfb1dba into main Aug 3, 2023
3 checks passed

kobayurii deleted the add_retry branch December 20, 2023 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement(rpc-server): Add additional logs for `shadow_data_consistency` #93

Improvement(rpc-server): Add additional logs for `shadow_data_consistency` #93

kobayurii commented Jul 20, 2023 •

edited

Loading

khorolets left a comment

telezhnaya left a comment

kobayurii commented Jul 21, 2023

khorolets left a comment

telezhnaya left a comment

kobayurii commented Jul 22, 2023

khorolets left a comment

telezhnaya left a comment

khorolets Jul 31, 2023

khorolets Jul 31, 2023

telezhnaya Jul 31, 2023

khorolets Aug 2, 2023

kobayurii Aug 2, 2023 •

edited

Loading

khorolets commented Aug 2, 2023

Improvement(rpc-server): Add additional logs for shadow_data_consistency #93

Improvement(rpc-server): Add additional logs for shadow_data_consistency #93

Conversation

kobayurii commented Jul 20, 2023 • edited Loading

khorolets left a comment

Choose a reason for hiding this comment

telezhnaya left a comment

Choose a reason for hiding this comment

kobayurii commented Jul 21, 2023

khorolets left a comment

Choose a reason for hiding this comment

telezhnaya left a comment

Choose a reason for hiding this comment

kobayurii commented Jul 22, 2023

khorolets left a comment

Choose a reason for hiding this comment

telezhnaya left a comment

Choose a reason for hiding this comment

khorolets Jul 31, 2023

Choose a reason for hiding this comment

khorolets Jul 31, 2023

Choose a reason for hiding this comment

telezhnaya Jul 31, 2023

Choose a reason for hiding this comment

khorolets Aug 2, 2023

Choose a reason for hiding this comment

kobayurii Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

khorolets commented Aug 2, 2023

Improvement(rpc-server): Add additional logs for `shadow_data_consistency` #93

Improvement(rpc-server): Add additional logs for `shadow_data_consistency` #93

kobayurii commented Jul 20, 2023 •

edited

Loading

kobayurii Aug 2, 2023 •

edited

Loading